Skip to content

Automated Test: group-dm-user-addition-post #328

Conversation

admin-coderabbit
Copy link
Owner

@admin-coderabbit admin-coderabbit commented Feb 4, 2026

This pull request was automatically created by @coderabbitai/e2e-reviewer.

Batch created pull request.

Summary by CodeRabbit

Release Notes

  • New Features

    • Added pagination controls (previous/next) for viewing group members
    • Added ability to add multiple members to groups
    • Added ability to remove individual members from groups with confirmation
  • Style

    • Improved layout and styling for group administration forms and member display area

@coderabbit-eval
Copy link

coderabbit-eval bot commented Feb 4, 2026

📝 Walkthrough

Walkthrough

This PR refactors group member management by shifting pagination logic from controllers to the Discourse.Group model, introducing new admin UI components for paginated member listing, and adding dedicated API endpoints for member operations (add/remove). The backend consolidates CRUD actions and the frontend integrates member management controls into a cohesive form.

Changes

Cohort / File(s) Summary
Admin Group Controller & Routes
app/assets/javascripts/admin/controllers/admin-group.js.es6, app/assets/javascripts/admin/routes/admin_group_route.js
Added computed pagination properties (currentPage, totalPages, showingFirst, showingLast), member management actions (next, previous, removeMember, addMembers), and property usernames. Simplified route setup by removing afterModel hook and consolidating member fetching into setupController.
Admin Group Template & Member Views
app/assets/javascripts/admin/templates/group.hbs, app/assets/javascripts/admin/templates/group_member.hbs, app/assets/javascripts/admin/views/group-member.js.es6
Restructured group edit form with integrated member list, pagination controls, and add-members section. Added new template and view for rendering individual group members with remove capability.
Group Model
app/assets/javascripts/discourse/models/group.js
Added pagination properties (limit, offset, user_count); refactored findMembers() to fetch paginated members; added addMembers() and removeMember() methods; introduced create() method; updated asJSON() to return simplified group data; refined save() and destroy() logic.
Public-Facing Group Routes & Templates
app/assets/javascripts/discourse/routes/group-members.js.es6, app/assets/javascripts/discourse/templates/group/members.hbs, app/assets/javascripts/discourse/templates/components/admin-group-selector.hbs, app/assets/javascripts/discourse/templates/user-selector-autocomplete.raw.hbs
Updated setupController signature to accept model parameter; changed member loop source from model to members; removed redundant input field from admin selector; improved template formatting without functional changes.
Backend Group Controller
app/controllers/admin/groups_controller.rb, app/controllers/groups_controller.rb
Added new actions: create, update, destroy, add_members, remove_member, refresh_automatic_groups. Implemented automatic-group protection for member modifications. Updated members action to return paginated results with metadata (total, limit, offset).
Routing Configuration
config/routes.rb
Replaced GET "users" collection route with PUT "members" (add_members) and DELETE "members" (remove_member) routes for admin group member management.
Styling
app/assets/stylesheets/common/admin/admin_base.scss
Added form-horizontal layout styling for groups and badges sections with label formatting, input widths, and navigation button states; removed existing form-horizontal block from section.details.
Translations & Tests
config/locales/client.en.yml, spec/controllers/admin/groups_controller_spec.rb
Updated group selector placeholder and added i18n entries for member management (delete_member_confirm, add, add_members). Reorganized controller specs into action-based contexts with tests for pagination, name normalization, automatic-group restrictions, and member operations.

Sequence Diagram(s)

sequenceDiagram
    participant Admin as Admin User
    participant UI as Admin UI
    participant Model as Group Model
    participant Server as Admin API

    Admin->>UI: Click "Next" pagination button
    UI->>Model: update offset, trigger next action
    Model->>Server: findMembers(limit, offset)
    Server->>Model: return paginated members + metadata
    Model->>UI: update members, currentPage, totalPages
    UI->>Admin: render updated member list

    Admin->>UI: Enter usernames & click "Add Members"
    UI->>Model: addMembers(usernames)
    Model->>Server: PUT /admin/groups/{id}/members
    Server->>Model: members added successfully
    Model->>Server: findMembers (reload)
    Server->>Model: return updated member list
    Model->>UI: update members display
    UI->>Admin: show updated group with new members
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 Hippity-hop, the members now page,
New endpoints tame the management stage,
Pagination buttons (next, prev with glee),
Add and remove flow smoothly and free!
Form-bound controls, a refactored delight,
Group member dance done proper and right!

🚥 Pre-merge checks | ✅ 1 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Title check ⚠️ Warning The PR title 'Automated Test: group-dm-user-addition-post' does not match the actual changeset, which refactors group member management UI, pagination, and API endpoints across multiple files. Update the title to reflect the main changes, such as 'Refactor group member management UI and API endpoints' or similar that accurately describes the substantial changes made.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch group-dm-user-addition-post

Comment @coderabbit eval help to get the list of available commands and usage tips.

@coderabbit-eval coderabbit-eval bot closed this Feb 4, 2026
Copy link

@coderabbit-eval coderabbit-eval bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/assets/javascripts/discourse/templates/group/members.hbs (1)

18-19: ⚠️ Potential issue | 🟡 Minor

Stray </div> closing tag without a matching opening <div>.

Line 19 has a </div> that doesn't correspond to any opening <div>. This appears pre-existing but will produce invalid HTML inside the <table>. Consider removing it.

Proposed fix
     </tr>
-    </div>
   {{/each}}
🤖 Fix all issues with AI agents
In `@app/assets/javascripts/admin/controllers/admin-group.js.es6`:
- Around line 11-14: The totalPages computed property
(totalPages.property("limit", "user_count")) incorrectly uses
Math.floor(this.get("user_count") / this.get("limit")) + 1 causing an extra page
when user_count is an exact multiple of limit; change the computation to use
Math.ceil(this.get("user_count") / this.get("limit")) and keep the zero-check
for user_count so totalPages returns 0 when user_count is 0.

In `@app/assets/javascripts/admin/templates/group.hbs`:
- Line 29: The Add button in the group.hbs template currently defaults to
type="submit" and will submit surrounding forms; update the <button> with the
action "addMembers" to explicitly include type="button" so clicking it won't
trigger form submission, leaving the class 'btn add', fa-icon "plus", and i18n
'admin.groups.add' unchanged.
- Around line 46-51: The two button elements that invoke the Ember actions
"save" and "destroy" default to type="submit" and will trigger native form
submission; update both buttons (the one with class 'btn btn-primary' invoking
the "save" action and the conditional 'btn btn-danger' invoking the "destroy"
action) to include type="button" so clicking them only runs the Ember actions
and prevents page reload/navigation.

In `@app/assets/javascripts/discourse/models/group.js`:
- Around line 40-60: Both removeMember and addMembers call self.findMembers()
inside their .then() callbacks but fail to return that promise, so callers
resolve before the member list finishes reloading; update the .then() handlers
in removeMember and addMembers to return self.findMembers() (i.e., change the
callback to return the promise from findMembers) so the promise chain waits for
the member-list refresh.

In `@app/assets/javascripts/discourse/routes/group-members.js.es6`:
- Around line 8-12: setupController currently calls model.findMembers() but does
not handle its returned promise; attach error handling by either returning the
promise from setupController (so the router can handle rejections) or chaining a
.catch(...) on model.findMembers() to handle/log failures. Specifically, in
setupController (which sets group showing and controller.model), change the call
to model.findMembers() to either return model.findMembers() or to call
model.findMembers().catch(error => { /* handle or log e.g.,
controller.set('loadError', error) or this.onError(error) */ }) so rejections
are not left unhandled.

In `@app/controllers/admin/groups_controller.rb`:
- Around line 6-9: The LIKE wildcards in the user-provided search string are not
escaped, so replace the raw interpolation with an escaped version using
ActiveRecord's sanitize_sql_like; e.g. convert params[:search] to a string, call
ActiveRecord::Base.sanitize_sql_like(search) (or sanitize_sql_like(search) if
available) to escape %/_ then use groups.where("name ILIKE ?", "%#{escaped}%")
so the search treats literal % and _ correctly.

In `@app/controllers/groups_controller.rb`:
- Around line 22-26: limit is unbounded and offset can be negative; clamp both
before querying: coerce params[:limit] to an integer, cap it to a safe maximum
(e.g., MAX_LIMIT constant or 100) and ensure it is at least 1, and coerce
params[:offset] to an integer and clamp it to a minimum of 0; then use these
sanitized values in the
group.users.order(:username_lower).limit(limit).offset(offset) query and keep
total = group.users.count unchanged.

In `@spec/controllers/admin/groups_controller_spec.rb`:
- Around line 112-130: The test for the remove_member action is using the wrong
HTTP verb (xhr :put) so it hits add_members; change the test that calls
remove_member to use DELETE (xhr :delete) so it exercises
GroupsController#remove_member and matches the route for "members" =>
"groups#remove_member"; update the spec example titled "cannot remove members
from automatic groups" to call xhr :delete, :remove_member with the same params.
🧹 Nitpick comments (8)
app/assets/stylesheets/common/admin/admin_base.scss (1)

471-476: Hardcoded colors break theming/dark mode support.

#333 and #aaa won't adapt to dark themes — text could become invisible against a dark background. Every other color reference in this file uses SCSS variables ($primary, $secondary, etc.) or helper functions. These should follow the same pattern.

♻️ Suggested fix using theme-aware variables
   .next, .previous {
-    color: `#333` !important;
+    color: $primary !important;
     &.disabled {
-      color: `#aaa` !important;
+      color: scale-color($primary, $lightness: 50%) !important;
     }
   }
app/assets/javascripts/discourse/models/group.js (2)

81-84: Extraneous semicolon after if block.

Line 82: if (!this.get('id')) { return }; — the trailing semicolon after } is an empty statement. It's harmless but syntactically unusual. Also, return without a value inside a function that otherwise returns a promise means callers of destroy() may get undefined instead of a promise when id is falsy.

Proposed cleanup
   destroy: function(){
-    if (!this.get('id')) { return };
+    if (!this.get('id')) { return; }
     return Discourse.ajax("/admin/groups/" + this.get('id'), {type: "DELETE"});
   },

20-38: Offset clamping allows offset === user_count, which yields empty results.

Line 23 clamps offset with Math.min(user_count, ...). When offset equals user_count (e.g., navigating past the last page), the API request will start at the boundary and return zero members. If this is intentional (the UI prevents it), this is fine — just noting the edge case.

spec/controllers/admin/groups_controller_spec.rb (1)

13-32: RuboCop: Extra empty lines at block body boundaries throughout the file.

Static analysis flagged numerous Layout/EmptyLinesAroundBlockBody violations (extra blank lines at the beginning/end of context blocks). Consider removing the extra blank lines for consistency if the project enforces this rule.

Also applies to: 34-47, 49-60, 62-78, 80-89, 91-110

app/controllers/admin/groups_controller.rb (2)

65-82: Silent skip when usernames don't exist — consider returning feedback.

When a username in the comma-separated list doesn't match any User, it is silently ignored (line 72-74). The caller receives a success response with no indication of which users were not found. This could confuse admins who mistype a username.

Consider collecting unmatched usernames and returning them in the response, or at minimum logging them.


71-75: RuboCop: Assignment in condition — wrap in parentheses to signal intent.

Line 72 uses if user = User.find_by_username(username) which RuboCop flags. Wrapping the assignment in parentheses makes intent explicit.

Proposed fix
-      if user = User.find_by_username(username)
+      if (user = User.find_by_username(username))
app/assets/javascripts/admin/controllers/admin-group.js.es6 (2)

6-9: currentPage formula is correct but uses loose equality.

The == 0 check at line 7 uses loose equality, which also matches null, undefined, and "". While this is a safe fallback here (returns 0 for unloaded data), prefer === 0 for explicitness if only the numeric zero case is intended.


61-65: TODO: Input should be cleared after adding members.

The TODO at line 62 notes that the usernames input isn't cleared after a successful add. Consider resetting it in the .then() callback of addMembers to prevent double-submission.

Do you want me to generate the fix to clear the input after a successful add?

Comment on lines +11 to +14
totalPages: function() {
if (this.get("user_count") == 0) { return 0; }
return Math.floor(this.get("user_count") / this.get("limit")) + 1;
}.property("limit", "user_count"),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Bug: totalPages formula is off-by-one for exact multiples.

Math.floor(user_count / limit) + 1 returns one extra page when user_count is evenly divisible by limit. For example, 100 users with limit 50 → Math.floor(2) + 1 = 3 pages instead of the correct 2.

Use Math.ceil instead:

🐛 Proposed fix
   totalPages: function() {
     if (this.get("user_count") == 0) { return 0; }
-    return Math.floor(this.get("user_count") / this.get("limit")) + 1;
+    return Math.ceil(this.get("user_count") / this.get("limit"));
   }.property("limit", "user_count"),
🤖 Prompt for AI Agents
In `@app/assets/javascripts/admin/controllers/admin-group.js.es6` around lines 11
- 14, The totalPages computed property (totalPages.property("limit",
"user_count")) incorrectly uses Math.floor(this.get("user_count") /
this.get("limit")) + 1 causing an extra page when user_count is an exact
multiple of limit; change the computation to use
Math.ceil(this.get("user_count") / this.get("limit")) and keep the zero-check
for user_count so totalPages returns 0 when user_count is 0.

<div>
<label for="user-selector">{{i18n 'admin.groups.add_members'}}</label>
{{user-selector usernames=usernames placeholderKey="admin.groups.selector_placeholder" id="user-selector"}}
<button {{action "addMembers"}} class='btn add'>{{fa-icon "plus"}} {{i18n 'admin.groups.add'}}</button>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Add button also needs type="button" to avoid form submission.

Same issue as the Save/Delete buttons — this <button> defaults to type="submit".

🐛 Proposed fix
-        <button {{action "addMembers"}} class='btn add'>{{fa-icon "plus"}} {{i18n 'admin.groups.add'}}</button>
+        <button type="button" {{action "addMembers"}} class='btn add'>{{fa-icon "plus"}} {{i18n 'admin.groups.add'}}</button>
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<button {{action "addMembers"}} class='btn add'>{{fa-icon "plus"}} {{i18n 'admin.groups.add'}}</button>
<button type="button" {{action "addMembers"}} class='btn add'>{{fa-icon "plus"}} {{i18n 'admin.groups.add'}}</button>
🤖 Prompt for AI Agents
In `@app/assets/javascripts/admin/templates/group.hbs` at line 29, The Add button
in the group.hbs template currently defaults to type="submit" and will submit
surrounding forms; update the <button> with the action "addMembers" to
explicitly include type="button" so clicking it won't trigger form submission,
leaving the class 'btn add', fa-icon "plus", and i18n 'admin.groups.add'
unchanged.

Comment on lines +46 to +51
<div class='buttons'>
<button {{action "save"}} {{bind-attr disabled="disableSave"}} class='btn btn-primary'>{{i18n 'admin.customize.save'}}</button>
{{#unless automatic}}
<button {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button>
{{/unless}}
</div>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Buttons inside <form> will trigger native form submission.

The <button> elements at lines 47 and 49 default to type="submit" since no type attribute is specified. Clicking them will trigger the browser's native form submission (page navigation/reload) in addition to the Ember {{action}}. This will likely cause the page to reload before the Ember action completes.

Add type="button" to both buttons to prevent this.

🐛 Proposed fix
-    <button {{action "save"}} {{bind-attr disabled="disableSave"}} class='btn btn-primary'>{{i18n 'admin.customize.save'}}</button>
+    <button type="button" {{action "save"}} {{bind-attr disabled="disableSave"}} class='btn btn-primary'>{{i18n 'admin.customize.save'}}</button>
     {{`#unless` automatic}}
-      <button {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button>
+      <button type="button" {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button>
     {{/unless}}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
<div class='buttons'>
<button {{action "save"}} {{bind-attr disabled="disableSave"}} class='btn btn-primary'>{{i18n 'admin.customize.save'}}</button>
{{#unless automatic}}
<button {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button>
{{/unless}}
</div>
<div class='buttons'>
<button type="button" {{action "save"}} {{bind-attr disabled="disableSave"}} class='btn btn-primary'>{{i18n 'admin.customize.save'}}</button>
{{`#unless` automatic}}
<button type="button" {{action "destroy"}} class='btn btn-danger'>{{fa-icon "trash-o"}}{{i18n 'admin.customize.delete'}}</button>
{{/unless}}
</div>
🤖 Prompt for AI Agents
In `@app/assets/javascripts/admin/templates/group.hbs` around lines 46 - 51, The
two button elements that invoke the Ember actions "save" and "destroy" default
to type="submit" and will trigger native form submission; update both buttons
(the one with class 'btn btn-primary' invoking the "save" action and the
conditional 'btn btn-danger' invoking the "destroy" action) to include
type="button" so clicking them only runs the Ember actions and prevents page
reload/navigation.

Comment on lines +40 to 60
removeMember: function(member) {
var self = this;
return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {
type: "DELETE",
data: { user_id: member.get("id") }
}).then(function() {
// reload member list
self.findMembers();
});
},

asJSON: function() {
return { group: {
name: this.get('name'),
alias_level: this.get('alias_level'),
visible: !!this.get('visible'),
usernames: this.get('usernames') } };
addMembers: function(usernames) {
var self = this;
return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {
type: "PUT",
data: { usernames: usernames }
}).then(function() {
// reload member list
self.findMembers();
})
},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

findMembers() promise not returned from .then() — callers can't await the member-list refresh.

In both removeMember and addMembers, the .then() callback calls self.findMembers() but doesn't return it. This means the promise returned to callers resolves before the member list has actually reloaded, leading to potential UI timing issues.

Proposed fix
   removeMember: function(member) {
     var self = this;
     return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {
       type: "DELETE",
       data: { user_id: member.get("id") }
     }).then(function() {
       // reload member list
-      self.findMembers();
+      return self.findMembers();
     });
   },

   addMembers: function(usernames) {
     var self = this;
     return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {
       type: "PUT",
       data: { usernames: usernames }
     }).then(function() {
       // reload member list
-      self.findMembers();
-    })
+      return self.findMembers();
+    });
   },
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
removeMember: function(member) {
var self = this;
return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {
type: "DELETE",
data: { user_id: member.get("id") }
}).then(function() {
// reload member list
self.findMembers();
});
},
asJSON: function() {
return { group: {
name: this.get('name'),
alias_level: this.get('alias_level'),
visible: !!this.get('visible'),
usernames: this.get('usernames') } };
addMembers: function(usernames) {
var self = this;
return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {
type: "PUT",
data: { usernames: usernames }
}).then(function() {
// reload member list
self.findMembers();
})
},
removeMember: function(member) {
var self = this;
return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {
type: "DELETE",
data: { user_id: member.get("id") }
}).then(function() {
// reload member list
return self.findMembers();
});
},
addMembers: function(usernames) {
var self = this;
return Discourse.ajax('/admin/groups/' + this.get('id') + '/members.json', {
type: "PUT",
data: { usernames: usernames }
}).then(function() {
// reload member list
return self.findMembers();
});
},
🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/models/group.js` around lines 40 - 60, Both
removeMember and addMembers call self.findMembers() inside their .then()
callbacks but fail to return that promise, so callers resolve before the member
list finishes reloading; update the .then() handlers in removeMember and
addMembers to return self.findMembers() (i.e., change the callback to return the
promise from findMembers) so the promise chain waits for the member-list
refresh.

Comment on lines +8 to 12
setupController: function(controller, model) {
this.controllerFor('group').set('showing', 'members');
controller.set("model", model);
model.findMembers();
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Unhandled promise from findMembers().

model.findMembers() returns a promise whose rejection is never caught. If the AJAX call fails (network error, 4xx/5xx), this will produce an unhandled promise rejection. Consider adding a .catch() or returning the promise so the router's error handling can intercept it.

Proposed fix
   setupController: function(controller, model) {
     this.controllerFor('group').set('showing', 'members');
     controller.set("model", model);
-    model.findMembers();
+    model.findMembers().catch(function() {
+      // optionally surface an error to the user
+    });
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
setupController: function(controller, model) {
this.controllerFor('group').set('showing', 'members');
controller.set("model", model);
model.findMembers();
}
setupController: function(controller, model) {
this.controllerFor('group').set('showing', 'members');
controller.set("model", model);
model.findMembers().catch(function() {
// optionally surface an error to the user
});
}
🤖 Prompt for AI Agents
In `@app/assets/javascripts/discourse/routes/group-members.js.es6` around lines 8
- 12, setupController currently calls model.findMembers() but does not handle
its returned promise; attach error handling by either returning the promise from
setupController (so the router can handle rejections) or chaining a .catch(...)
on model.findMembers() to handle/log failures. Specifically, in setupController
(which sets group showing and controller.model), change the call to
model.findMembers() to either return model.findMembers() or to call
model.findMembers().catch(error => { /* handle or log e.g.,
controller.set('loadError', error) or this.onError(error) */ }) so rejections
are not left unhandled.

Comment on lines 6 to 9
if search = params[:search]
search = search.to_s
groups = groups.where("name ilike ?", "%#{search}%")
groups = groups.where("name ILIKE ?", "%#{search}%")
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

LIKE wildcards (%, _) in search are not escaped.

A search term like % or _ will act as a LIKE wildcard, matching more broadly than expected. This is admin-only so the risk is low, but it's a correctness gap.

🛡️ Proposed fix — sanitize LIKE wildcards
     if search = params[:search]
       search = search.to_s
-      groups = groups.where("name ILIKE ?", "%#{search}%")
+      groups = groups.where("name ILIKE ?", "%#{ActiveRecord::Base.sanitize_sql_like(search)}%")
     end
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if search = params[:search]
search = search.to_s
groups = groups.where("name ilike ?", "%#{search}%")
groups = groups.where("name ILIKE ?", "%#{search}%")
end
if search = params[:search]
search = search.to_s
groups = groups.where("name ILIKE ?", "%#{ActiveRecord::Base.sanitize_sql_like(search)}%")
end
🧰 Tools
🪛 RuboCop (1.84.0)

[warning] 6-6: Use == if you meant to do a comparison or wrap the expression in parentheses to indicate you meant to assign in a condition.

(Lint/AssignmentInCondition)

🤖 Prompt for AI Agents
In `@app/controllers/admin/groups_controller.rb` around lines 6 - 9, The LIKE
wildcards in the user-provided search string are not escaped, so replace the raw
interpolation with an escaped version using ActiveRecord's sanitize_sql_like;
e.g. convert params[:search] to a string, call
ActiveRecord::Base.sanitize_sql_like(search) (or sanitize_sql_like(search) if
available) to escape %/_ then use groups.where("name ILIKE ?", "%#{escaped}%")
so the search treats literal % and _ correctly.

Comment on lines +22 to +26
limit = (params[:limit] || 50).to_i
offset = params[:offset].to_i

total = group.users.count
members = group.users.order(:username_lower).limit(limit).offset(offset)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Cap limit and clamp offset to prevent abuse and negative values.

limit is unbounded — a client can pass limit=1000000 and dump the entire user list in one request. Also, a negative offset (e.g., offset=-5) passes .to_i and reaches ActiveRecord, which may produce unexpected behavior depending on the database adapter.

🛡️ Proposed fix
-    limit = (params[:limit] || 50).to_i
-    offset = params[:offset].to_i
+    limit = [(params[:limit] || 50).to_i, 200].min
+    offset = [params[:offset].to_i, 0].max
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
limit = (params[:limit] || 50).to_i
offset = params[:offset].to_i
total = group.users.count
members = group.users.order(:username_lower).limit(limit).offset(offset)
limit = [(params[:limit] || 50).to_i, 200].min
offset = [params[:offset].to_i, 0].max
total = group.users.count
members = group.users.order(:username_lower).limit(limit).offset(offset)
🤖 Prompt for AI Agents
In `@app/controllers/groups_controller.rb` around lines 22 - 26, limit is
unbounded and offset can be negative; clamp both before querying: coerce
params[:limit] to an integer, cap it to a safe maximum (e.g., MAX_LIMIT constant
or 100) and ensure it is at least 1, and coerce params[:offset] to an integer
and clamp it to a minimum of 0; then use these sanitized values in the
group.users.order(:username_lower).limit(limit).offset(offset) query and keep
total = group.users.count unchanged.

Comment on lines +112 to 130
context ".remove_member" do

it "cannot remove members from automatic groups" do
xhr :put, :remove_member, group_id: 1, user_id: 42
response.status.should == 422
end

it "is able to remove a member" do
group = Fabricate(:group)
user = Fabricate(:user)
group.add(user)
group.save

xhr :delete, :remove_member, group_id: group.id, user_id: user.id

response.should be_success
group.reload
group.users.count.should == 0
end

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Find routes configuration
fd "routes\.rb" --type f | head -20

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 104


🏁 Script executed:

# Look for remove_member in routes
rg "remove_member" --type rb

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 114


🏁 Script executed:

# Check the admin groups controller for remove_member action
fd "groups_controller\.rb" --type f | grep -E "admin|app/controllers" | head -5

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 167


🏁 Script executed:

# Check the routes configuration for remove_member
rg "remove_member" config/routes.rb -A 2 -B 2

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 275


🏁 Script executed:

# Check the admin groups controller for remove_member
cat app/controllers/admin/groups_controller.rb | head -200

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 2348


🏁 Script executed:

# Get file size first to determine how to read it
wc -l app/controllers/admin/groups_controller.rb

Repository: admin-coderabbit/coderabbit-discourse-mirror

Length of output: 134


Inconsistent HTTP method for remove_member test.

Line 115 uses xhr :put, :remove_member but the route configuration shows delete "members" => "groups#remove_member". The DELETE method is required; the PUT request will hit the wrong route (add_members). Update line 115 to use DELETE:

Fix
     it "cannot remove members from automatic groups" do
-      xhr :put, :remove_member, group_id: 1, user_id: 42
+      xhr :delete, :remove_member, group_id: 1, user_id: 42
       response.status.should == 422
     end
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
context ".remove_member" do
it "cannot remove members from automatic groups" do
xhr :put, :remove_member, group_id: 1, user_id: 42
response.status.should == 422
end
it "is able to remove a member" do
group = Fabricate(:group)
user = Fabricate(:user)
group.add(user)
group.save
xhr :delete, :remove_member, group_id: group.id, user_id: user.id
response.should be_success
group.reload
group.users.count.should == 0
end
context ".remove_member" do
it "cannot remove members from automatic groups" do
xhr :delete, :remove_member, group_id: 1, user_id: 42
response.status.should == 422
end
it "is able to remove a member" do
group = Fabricate(:group)
user = Fabricate(:user)
group.add(user)
group.save
xhr :delete, :remove_member, group_id: group.id, user_id: user.id
response.should be_success
group.reload
group.users.count.should == 0
end
🧰 Tools
🪛 RuboCop (1.84.0)

[convention] 113-114: Extra empty line detected at block body beginning.

(Layout/EmptyLinesAroundBlockBody)

🤖 Prompt for AI Agents
In `@spec/controllers/admin/groups_controller_spec.rb` around lines 112 - 130, The
test for the remove_member action is using the wrong HTTP verb (xhr :put) so it
hits add_members; change the test that calls remove_member to use DELETE (xhr
:delete) so it exercises GroupsController#remove_member and matches the route
for "members" => "groups#remove_member"; update the spec example titled "cannot
remove members from automatic groups" to call xhr :delete, :remove_member with
the same params.

Sign in to join this conversation on GitHub.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant